Skip to content

Add lookup_table_v2 BF16 op#33172

Merged
jczaja merged 5 commits intoPaddlePaddle:developfrom
wozna:bf16_lookup_table_v2
Jun 16, 2021
Merged

Add lookup_table_v2 BF16 op#33172
jczaja merged 5 commits intoPaddlePaddle:developfrom
wozna:bf16_lookup_table_v2

Conversation

@wozna
Copy link

@wozna wozna commented May 27, 2021 •
edited
Loading

PR types

New features

PR changes

OPs

Describe

This PR adds BF16 support for FWD and BWD lookup_table_v2 op.
Added UT that reuses test test_lookup_table_bf16_op.

@paddle-bot-old
Copy link

paddle-bot-old bot commented May 27, 2021

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@wozna wozna requested a review from arogowie-intel June 1, 2021 10:22
arogowie-intel
arogowie-intel reviewed Jun 1, 2021
View reviewed changes
python/paddle/fluid/tests/unittests/test_lookup_table_bf16_op.py Outdated


def _lookup(weights, ids, flat_ids):
def _lookup(weights, ids, flat_ids, op_type="lookup_table"):
Copy link
Contributor

@arogowie-intel arogowie-intel Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "op_version" would be better? Now it suggests that this function is for different operator types.

@jczaja jczaja self-requested a review June 2, 2021 17:49
jczaja
jczaja previously approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@jczaja jczaja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

arogowie-intel
arogowie-intel previously approved these changes Jun 7, 2021
View reviewed changes
@lidanqing-vv
Copy link
Contributor

lidanqing-vv commented Jun 9, 2021

@wozna

2021-06-02 16:12:47 2021-06-02 16:12:47 (2.35 MB/s) - 已保存 “bk.txt” [5/5])
2021-06-02 16:12:54 ****************
2021-06-02 16:12:54 0. Developers are not allowed to set the check_dygraph field directly, which is set to True by default. If you need to change the check_dygraph field, you must have one RD (phlrain (Recommend), fuyinno4 (Recommend for kunlun) or lanxianghit) review and approve. 
2021-06-02 16:12:54 The code that do not meet the specification are as follows:
2021-06-02 16:12:54  python/paddle/fluid/tests/unittests/test_lookup_table_v2_bf16_op.py : 
2021-06-02 16:12:54 +        self.check_output_with_place(core.CPUPlace(), check_dygraph=False)
2021-06-02 16:12:54 +        self.check_output_with_place(core.CPUPlace(), check_dygraph=False) 
2021-06-02 16:12:54 1. It is an Op accuracy problem, please take care of it. You must have one RD (zhangting2020 (Recommend), luotao1 or phlrain) approval for the usage (either add or delete) of @skip_check_grad_ci. For more information, please refer to: https://github.com/PaddlePaddle/Paddle/wiki/Gradient-Check-Is-Required-for-Op-Test. The corresponding lines are as follows:

Hi, here only approval needed or something else need to be fixed?

@wozna
Copy link
Author

wozna commented Jun 9, 2021

@lidanqing-intel Yes, only this approval is needed.

@lidanqing-vv
Copy link
Contributor

lidanqing-vv commented Jun 9, 2021

@wozna Please try to set self.is_bfloat16_op() or set op datatype to bf16, to avoid the approval

@wozna wozna dismissed stale reviews from arogowie-intel and jczaja via e0bb318 June 9, 2021 14:49
@wozna
Copy link
Author

wozna commented Jun 9, 2021

@wozna Please try to set self.is_bfloat16_op() or set op datatype to bf16, to avoid the approval

I removed that, but still, approval for skip_check_grad_ci is needed:

@skip_check_grad_ci(
reason="Since paddings are not trainable and fixed in forward,"
"the gradient of paddings makes no sense and we don't "   
 "test the gradient here.")

jczaja
jczaja previously approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@jczaja jczaja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jczaja jczaja requested a review from arlesniak June 14, 2021 08:29
arlesniak
arlesniak previously approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@arlesniak arlesniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jczaja
Copy link
Contributor

jczaja commented Jun 14, 2021

@wzzju Could you please start your review? PR-CI-APPROVAL needs approval to pass.

@lidanqing-vv
Copy link
Contributor

lidanqing-vv commented Jun 15, 2021

If approval is too difficult to get, we can only try to use something similar to pass like in test_conv2d_op.py
image

@wozna wozna dismissed stale reviews from arlesniak and jczaja via a00c2b3 June 16, 2021 09:27
@wozna
Copy link
Author

wozna commented Jun 16, 2021 •
edited
Loading

Initially, I used @skip_check_grad_ci exactly as it was in the base tests like in lookup_table_v2_op.py. It turned out that even if padding is not used in BWD, checking the gradient in this test works correctly, so I removed that.

@jczaja jczaja merged commit 9d6c8bd into PaddlePaddle:develop Jun 16, 2021
@lidanqing-vv lidanqing-vv mentioned this pull request Jun 30, 2021
Merged
@wozna wozna deleted the bf16_lookup_table_v2 branch February 24, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@jczaja jczaja jczaja approved these changes

@arlesniak arlesniak arlesniak left review comments

@arogowie-intel arogowie-intel arogowie-intel left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wzzju wzzju

Labels

BF16 Intel

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wozna @lidanqing-vv @jczaja @arlesniak @arogowie-intel @wzzju